[staking] ActiveCandidate Exclude Candidate with Expired Endorsement #4062

envestcc · 2024-01-19T05:31:17Z

Description

Since the SelfStake field of a Candidate is not cleared when an endorsement expires, the current ActiveCandidate function does not remove expired candidates. This PR is aimed at fixing this issue.

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

[] make test
[] fullsync
[] Other test (please specify)

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Checklist:

[] My code follows the style guidelines of this project
[] I have performed a self-review of my code
[] I have commented my code, particularly in hard-to-understand areas
[] I have made corresponding changes to the documentation
[] My changes generate no new warnings
[] I have added tests that prove my fix is effective or that my feature works
[] New and existing unit tests pass locally with my changes
[] Any dependent changes have been merged and published in downstream modules

codecov · 2024-01-19T05:45:12Z

Codecov Report

Attention: Patch coverage is 58.33333% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 76.40%. Comparing base (e1f0636) to head (46e75e5).
Report is 192 commits behind head on master.

Files	Patch %	Lines
action/protocol/staking/protocol.go	60.00%	8 Missing ⚠️
action/protocol/staking/candidate_statereader.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4062      +/-   ##
==========================================
+ Coverage   75.38%   76.40%   +1.02%     
==========================================
  Files         303      340      +37     
  Lines       25923    28984    +3061     
==========================================
+ Hits        19541    22145    +2604     
- Misses       5360     5734     +374     
- Partials     1022     1105      +83

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CoderZhi · 2024-01-20T10:10:20Z

action/protocol/staking/protocol.go

+	}
+	// bucket is not endorsed to the candidate
+	if !address.Equal(vb.Candidate, cand.Owner) {
+		return false, nil


if this happens, there must be something wrong with our implementation?

indeed, if clearing candidates is implemented after bucket endorsed to others.

CoderZhi · 2024-01-20T10:11:58Z

action/protocol/staking/protocol.go

+		// other error
+		return false, err
+	default:
+		// endorsement does not exist


return false, nil or return false, ErrNotExist?

it is unexpected behaviours, should return error

CoderZhi · 2024-01-20T10:16:34Z

action/protocol/staking/protocol.go

+		// endorsement exists and expired before end of next epoch
+		rp := rolldpos.MustGetProtocol(protocol.MustGetRegistry(ctx))
+		currentEpochNum := rp.GetEpochNum(srHeight)
+		if endorse.Status(rp.GetEpochLastBlockHeight(currentEpochNum+1)) == EndorseExpired {


can you elaborate it?

It is checking that only nodes whose endorsement have not expired before the end of the next epoch will be considered as consensus nodes. This ensures that self-stake bucket is locked when participate in consensus, which facilitates the introduction of penalty mechanisms in the future.

CoderZhi · 2024-01-24T13:47:50Z

action/protocol/staking/protocol.go

@@ -513,6 +509,7 @@ func (p *Protocol) isActiveCandidate(ctx context.Context, csr CandidateStateRead
 		return false, err
 	default:
 		// endorsement does not exist
+		return false, errors.New("endorsement does not exist")


global variable

CoderZhi · 2024-01-24T14:13:38Z

action/protocol/staking/protocol.go

+	vb, err := csr.getBucket(cand.SelfStakeBucketIdx)
+	if err != nil {
+		if errors.Is(err, state.ErrStateNotExist) {
+			return false, nil


SelfStakeBucketIndex shouldn't be an invalid one, so we may always return false, err

Liuhaai · 2024-01-25T07:09:00Z

action/protocol/context.go

@@ -116,6 +116,7 @@ type (
 		SharedGasWithDapp                       bool
 		ExecutionSizeLimit32KB                  bool
 		UseZeroNonceForFreshAccount             bool
+		DisableDelegateEndorsement              bool


why DisableDelegateEndorsement not EnableDelegateEndorsement?

I prefer the default value for this bool represents the activated state of the new feature, while a default value for bool in Go is false.

Liuhaai · 2024-01-25T07:23:35Z

action/protocol/staking/protocol.go

@@ -492,7 +538,11 @@ func (p *Protocol) ActiveCandidates(ctx context.Context, sr protocol.StateReader
 			}
 			list[i].Votes.Add(list[i].Votes, contractVotes)
 		}
-		if list[i].SelfStake.Cmp(p.config.RegistrationConsts.MinSelfStake) >= 0 {
+		active, err := p.isActiveCandidate(ctx, c, list[i])


why are unactive cands included in L527?

There are two scenarios:

The self-stake bucket has been unstaked.

The endorsement has expired.

Both situations belong to a registered delegate, but they cannot participate in consensus due to a lack of staking.

ok. It seems the state of candidates aren't tracked in the db(active/unactive). Pls add ur comments to explain why the list is filtered above L541

sonarcloud · 2024-02-05T02:26:56Z

Quality Gate failed

Failed conditions

4.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

dustinxie · 2024-02-07T00:15:47Z

action/protocol/staking/protocol.go

+		if cand.SelfStake.Cmp(p.config.RegistrationConsts.MinSelfStake) < 0 {
+			return false, nil
+		}
+		return true, nil


just return cand.SelfStake.Cmp(p.config.RegistrationConsts.MinSelfStake) >= 0, nil

dustinxie · 2024-02-07T00:16:34Z

action/protocol/staking/protocol.go

+	}
+	rp := rolldpos.MustGetProtocol(protocol.MustGetRegistry(ctx))
+	currentEpochNum := rp.GetEpochNum(srHeight)
+	return csr.IsActiveCandidateAt(cand, rp.GetEpochLastBlockHeight(currentEpochNum+1))


why should be currentEpochNum+1?

refer to #4062 (comment)

dustinxie · 2024-02-07T00:19:14Z

action/protocol/staking/protocol.go

+		if err != nil {
+			return nil, err
+		}
+		if active {


from the usage here, it is checking current height, not currentEpochNum+1 height

dustinxie · 2024-02-07T00:20:59Z

action/protocol/staking/candidate_statereader.go

+		return false, nil
+	}
+	esr := NewEndorsementStateReader(c.SR())
+	endorse, err := esr.Get(cand.SelfStakeBucketIdx)


this should be guarded by hard-fork flag? before 1.14, no such thing as endorsement exist

hard-fork at https://github.com/iotexproject/iotex-core/pull/4062/files#diff-6bac7263ac5bdd5a72fb2bbb66bff76ebd0b794b0f998ed62bfafab7bba319e6R479

sonarcloud · 2024-03-11T00:46:20Z

Quality Gate failed

Failed conditions
5.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

envestcc requested review from CoderZhi, dustinxie and Liuhaai as code owners January 19, 2024 05:31

envestcc changed the title ~~[staking] ActiveCandidate Exclude Expired Endorsement~~ [staking] ActiveCandidate Exclude Candidate with Expired Endorsement Jan 19, 2024

CoderZhi reviewed Jan 20, 2024

View reviewed changes

CoderZhi approved these changes Jan 24, 2024

View reviewed changes

Liuhaai reviewed Jan 25, 2024

View reviewed changes

envestcc force-pushed the pr-activecand branch from d646bb2 to 22da849 Compare January 30, 2024 13:31

Liuhaai approved these changes Feb 5, 2024

View reviewed changes

dustinxie reviewed Feb 7, 2024

View reviewed changes

envestcc added 5 commits March 7, 2024 17:08

ActiveCandidate consider endorsement

84e3158

address comments

fdd2b62

address comment

bd408d5

refactor

ae5e699

reuse isSelfStakeBucket

55a48e1

envestcc force-pushed the pr-activecand branch from 4a693f6 to 55a48e1 Compare March 7, 2024 12:24

envestcc and others added 5 commits March 8, 2024 11:39

introduce CandidateStateCommon

77bbf07

Merge branch 'master' into pr-activecand

392a8c4

Merge branch 'master' into pr-activecand

1a03f4c

fix test

96663e0

Merge branch 'master' into pr-activecand

46e75e5

envestcc merged commit ef4ae12 into iotexproject:master Mar 11, 2024
2 of 5 checks passed

envestcc deleted the pr-activecand branch March 11, 2024 01:14

BrewTestBot mentioned this pull request Mar 25, 2024

ioctl 1.14.0 Homebrew/homebrew-core#167116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[staking] ActiveCandidate Exclude Candidate with Expired Endorsement #4062

[staking] ActiveCandidate Exclude Candidate with Expired Endorsement #4062

envestcc commented Jan 19, 2024

codecov bot commented Jan 19, 2024 •

edited

Loading

CoderZhi Jan 20, 2024

envestcc Jan 22, 2024 •

edited

Loading

CoderZhi Jan 20, 2024

envestcc Jan 22, 2024

CoderZhi Jan 20, 2024

envestcc Jan 22, 2024 •

edited

Loading

CoderZhi Jan 24, 2024

CoderZhi Jan 24, 2024

Liuhaai Jan 25, 2024

envestcc Jan 25, 2024

Liuhaai Jan 25, 2024

envestcc Jan 25, 2024

Liuhaai Feb 5, 2024

sonarcloud bot commented Feb 5, 2024

dustinxie Feb 7, 2024

dustinxie Feb 7, 2024

envestcc Mar 7, 2024

dustinxie Feb 7, 2024 •

edited

Loading

dustinxie Feb 7, 2024

envestcc Mar 7, 2024

sonarcloud bot commented Mar 11, 2024

[staking] ActiveCandidate Exclude Candidate with Expired Endorsement #4062

[staking] ActiveCandidate Exclude Candidate with Expired Endorsement #4062

Conversation

envestcc commented Jan 19, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

codecov bot commented Jan 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

envestcc Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

envestcc Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Feb 5, 2024

Quality Gate failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinxie Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 11, 2024

Quality Gate failed

codecov bot commented Jan 19, 2024 •

edited

Loading

envestcc Jan 22, 2024 •

edited

Loading

envestcc Jan 22, 2024 •

edited

Loading

dustinxie Feb 7, 2024 •

edited

Loading